Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Suggest dereferencing on assignment to mutable borrow #61054

Merged
merged 2 commits into from
May 24, 2019
Merged

Suggest dereferencing on assignment to mutable borrow #61054

merged 2 commits into from
May 24, 2019

Conversation

estebank
Copy link
Contributor

Fix #33570

@rust-highfive
Copy link
Collaborator

r? @zackmdavis

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 22, 2019
@estebank
Copy link
Contributor Author

cc @Centril on wording

@zackmdavis
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented May 22, 2019

📌 Commit 1e3302d has been approved by zackmdavis

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 22, 2019
@mark-i-m
Copy link
Member

Won't this suggestion trigger even if the type of the pointee and the assignee are different?

let mut x = 0;

(&mut x) = None; // suggestion to consider dereferencing does not work

@estebank
Copy link
Contributor Author

I'll add a negative check to the test to avoid regressions, but by the time we hit this code we have already checked that the two types can be unified and that the left is a mutable borrow.

@Centril
Copy link
Contributor

Centril commented May 23, 2019

@bors r- Until that test is added then :)

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels May 23, 2019
Centril
Centril suggested changes May 23, 2019
View reviewed changes
Copy link
Contributor

@Centril Centril left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a big, big fan of having a diagnostic for this as I make this mistake on a regular basis.

src/librustc_typeck/check/demand.rs
@@ -397,6 +398,29 @@ impl<'a, 'gcx, 'tcx> FnCtxt<'a, 'gcx, 'tcx> {
} else {
String::new()
};
if let Some(hir::Node::Expr(hir::Expr {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With this addition the function is ~200+ LOC -- can we avoid making it bigger? =P

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm focusing on slimming/splitting Parser now. I'll move to other parts of the compiler once in done with that.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool, just don't forget about it... =)

@estebank
Copy link
Contributor Author

@bors r=zackmdavis added the test

@bors
Copy link
Contributor

bors commented May 23, 2019

🙀 added is not a valid commit SHA. Please try again with 7fbbcfaafd08023cc2296481425e16a02ccd3f1a.

@estebank
Copy link
Contributor Author

@bors r=zackmdavis

@bors
Copy link
Contributor

bors commented May 23, 2019

📌 Commit 7fbbcfa has been approved by zackmdavis

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels May 23, 2019
@estebank
Copy link
Contributor Author

For the case in @mark-i-m's comment, the output is:

error[E0308]: mismatched types
 --> file2.rs:4:12
  |
4 | (&mut x) = None;
  |            ^^^^ expected mutable reference, found enum `std::option::Option`
  |
  = note: expected type `&mut {integer}`
             found type `std::option::Option<_>`

error[E0070]: invalid left-hand side expression
 --> file2.rs:4:1
  |
4 | (&mut x) = None;
  | ^^^^^^^^^^^^^^^ left-hand of expression not valid

Centril added a commit to Centril/rust that referenced this pull request May 23, 2019
@Centril
Rollup merge of rust-lang#61054 - estebank:mut-ref-reassign, r=zackmd…
b9459e7 
…avis

Suggest dereferencing on assignment to mutable borrow

Fix rust-lang#33570
@Centril Centril mentioned this pull request May 23, 2019
Merged
bors added a commit that referenced this pull request May 24, 2019
@bors
Auto merge of #61105 - Centril:rollup-t9lemjf, r=Centril
fd8e23c 
Rollup of 6 pull requests

Successful merges:

 - #59545 (Use arenas to avoid Lrc in queries #2)
 - #61054 (Suggest dereferencing on assignment to mutable borrow)
 - #61056 (tweak discriminant on non-nullary enum diagnostic)
 - #61082 (fix dangling reference in Vec::append)
 - #61086 (Box::into_unique: do the reborrow-to-raw *after* destroying the Box)
 - #61098 (Fix overflowing literal lint in loops)

Failed merges:

r? @ghost
@bors bors merged commit 7fbbcfa into rust-lang:master May 24, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Centril Centril Centril requested changes

Assignees

@zackmdavis zackmdavis

Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Compiler should hint to user to deref when doing reference assignment
6 participants
@estebank @rust-highfive @zackmdavis @bors @mark-i-m @Centril